Skip to content

Conversation

@labkey-matthewb
Copy link
Contributor

Rationale

For Array type required does not mean "IS NOT NULL" it means "cardinality() > 0".
Also for Multiple-choice text we prefer that null/"" converts to [] not to null.

To help implement this we consolidate column conversion into ColumnInfo.convert() and required/non-empty checking into JdbcType.isEmpty().

Related Pull Requests

Changes

return new SqlSelector(ExperimentService.get().getSchema(), blankRowsSQL).exists();
}

if (getTotalAndNonBlankSql(domain, prop, allRowsSQL, nonBlankRowsSQL))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For non-provisioned domain, why do we need a second check if getBlanksSql(domain, prop, blankRowsSQL) is false?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there may be a case where the domain supports provisioning (null != getStorageSchemaName()), but no table has been created yet (null == domain.getStorageTableName()).

In that case both methods could return false. I just tried to keep the flow mostly the same.

Comment on lines 1347 to 1350
} final String name = col.getName();



Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep should remove

@Override
public boolean isEmpty(Object value)
{
if (null == value)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we check for JSONArray to be consistent with other places?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants